-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: types.HeaderHooks
for RLP overrides
#89
Conversation
defer TestOnlyClearRegisteredExtras() | ||
|
||
extras := RegisterExtras[stubHeaderHooks, *stubHeaderHooks, struct{}]() | ||
rng := ethtest.NewPseudoRand(13579) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason for 13579? Or just smashed digits on the keyboard? ⌨️ I feel we should really just seed everything with 0
- if we want to test with various random values, we should fuzz instead 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel we should really just seed everything with
0
If there's a particular reason then I'm happy to consider it as the only reason I choose numbers is for some fun. Practically I think it's all the same, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does look like a magic number although it's not, at least with 0
it does look like a this-number-does-not-matter-see-I-m-using-zero. My mind might had been trained too much by the https://github.com/tommy-muehle/go-mnd linter to be fair 😄 What do you think?
Maybe we could address those in a chore PR later finding all numbers with a regex, I can create a super low priority issue for this 😸
Co-authored-by: Quentin McGaw <[email protected]> Signed-off-by: Arran Schlosberg <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]> Signed-off-by: Arran Schlosberg <[email protected]>
Co-authored-by: Quentin McGaw <[email protected]> Signed-off-by: Arran Schlosberg <[email protected]>
## Why this should be merged JSON equivalent of #89. ## How this works The check for registered extras, previously used in `{En,De}codeRLP()` methods is abstracted into a `Header.hooks() HeaderHooks` method that either returns (a) an instance of the registered type or (b) a `NOOPHeaderHooks` if no registration was performed. This is then used for all hooks, new (JSON) and old (RLP). ## How this was tested Extension of existing unit tests.
Why this should be merged
The
types.Header
fields of bothcoreth
andsubnet-evm
have been modified such that their RLP encodings (i.e. block hashes) aren't compatible with vanillageth
nor each other. This PR adds support for arbitrary RLP encoding coupled with type-safe extra payloads.How this works
Equivalent to #1 (
params
) and #44 (types.StateAccount
) registration of pseudo-generic payloads. The only major difference is the guarantee of a non-nil payload pointer, which means that the payload hooks are never called on nil pointers as this would make it difficult to decode RLP into them.How this was tested
Round-trip RLP {en,de}coding via a registered stub hook.